Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change to receive files outside the classpath in mongodb liquibase #39680

Merged
merged 1 commit into from
Mar 29, 2024
Merged

Change to receive files outside the classpath in mongodb liquibase #39680

merged 1 commit into from
Mar 29, 2024

Conversation

juanjogv
Copy link
Contributor

@juanjogv juanjogv commented Mar 25, 2024 •
edited by geoand
Loading

Due to the nature in which the resourceAccessor was created and injected when instantiating the Liquibase class
in LiquibaseMongodbFactory.createLiquibase()

try (ClassLoaderResourceAccessor resourceAccessor = new ClassLoaderResourceAccessor(Thread.currentThread().getContextClassLoader()))

it was not possible to pass a changeLog outside the classpath via liquibase-mongodb.change-log property. For people like me who want to use the quarkus-liquibase-mongodb extension only with the dev and test profiles in conjunction with the devservices and in productive environments use the liquibase cli directly this implementation is a bit problematic.

I also took the opportunity to implement the search-path property described in: docs.liquibase which allows me to limit the scope of the DirectoryResourceAccessor and not give it the default value of DirectoryResourceAccessor(Paths.get("/")).

@juanjogv juanjogv mentioned this pull request Mar 25, 2024
Closed
@geoand geoand requested review from loicmathieu and famod March 26, 2024 07:16
gsmet
gsmet reviewed Mar 26, 2024
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this pull request! I like what you did here and bonus point for making sure we weren't breaking the previous behavior.

Any chance you could have a look if the standard Liquibase could use the same treatment (it could be done in another PR)?

Also, I added a few very small comments.

...base-mongodb/runtime/src/main/java/io/quarkus/liquibase/mongodb/LiquibaseMongodbFactory.java Outdated Show resolved Hide resolved
...base-mongodb/runtime/src/main/java/io/quarkus/liquibase/mongodb/LiquibaseMongodbFactory.java Outdated Show resolved Hide resolved
@juanjogv
Copy link
Contributor Author

Thanks a lot for this pull request! I like what you did here and bonus point for making sure we weren't breaking the previous behavior.

Any chance you could have a look if the standard Liquibase could use the same treatment (it could be done in another PR)?

Also, I added a few very small comments.

I just uploaded a commit with the changes you suggested. Now about checking the standard Liquibase, sure! no problem, as soon as I have some free time I will check the code to make sure that the same treatment is done to the changelog.

@juanjogv juanjogv requested a review from gsmet March 26, 2024 21:06
@juanjogv juanjogv mentioned this pull request Mar 26, 2024
Merged
@juanjogv
Copy link
Contributor Author

Already did the PR in the standard Liquibase extension.

loicmathieu
loicmathieu approved these changes Mar 27, 2024
View reviewed changes
Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

As I saw that @gsmet already look at it I'll let him merge.

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Mar 28, 2024
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@juanjogv juanjogv requested a review from geoand March 28, 2024 18:31
@geoand
Copy link
Contributor

geoand commented Mar 29, 2024

Thanks for this!

I would like to have (at least) the last two commits squashed into the previous ones - FWIW if I made this change I would probably just make it one commit

@geoand geoand removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Mar 29, 2024
@juanjogv
Copy link
Contributor Author

Ready the squash of the last two commits into a single one.

@geoand
Copy link
Contributor

geoand commented Mar 29, 2024

I looked at the commits again and actually I think it makes a lot more sense to have a single one since are all about one new feature that is continuously revised. Those revisions make sense when developing, but they don't add much once the feature is complete.

Thereofore. can you please squash into a single commit? Once that's done we can merge (assuming CI is all green)

@juanjogv
Copy link
Contributor Author

Ready the squash of all commits into a single one.

geoand
geoand approved these changes Mar 29, 2024
View reviewed changes
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Mar 29, 2024
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@juanjogv juanjogv requested a review from geoand March 29, 2024 17:15
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@juanjogv
Change to receive files outside the classpath
7297a5f 
Changing contais to startsWith

Implementing CompositeResourceAccessor and the search-path property

Setting default value of the search path and an error in Paths.get()

Improving validation with search path default value

making suggested improvements

formatting code

sorting imports

Cleaning code
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Mar 29, 2024 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 7297a5f.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@geoand geoand merged commit b3ede8e into quarkusio:main Mar 29, 2024
19 checks passed
@quarkus-bot quarkus-bot bot added kind/enhancement New feature or request and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Mar 29, 2024
@quarkus-bot quarkus-bot bot added this to the 3.10 - main milestone Mar 29, 2024
@juanjogv
Copy link
Contributor Author

@geoand can you help me with this PR? is just a replication of the things done here into the standard liquibase extension.

@geoand
Copy link
Contributor

geoand commented Mar 29, 2024 •
edited
Loading

I'll have a look next week.

Thanks for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loicmathieu loicmathieu loicmathieu approved these changes

@famod famod Awaiting requested review from famod

@gsmet gsmet Awaiting requested review from gsmet

@geoand geoand Awaiting requested review from geoand

Assignees
No one assigned
Labels
area/liquibase area/mongodb kind/enhancement New feature or request
Projects
None yet
Milestone
3.10.0.CR1
Development

Successfully merging this pull request may close these issues.

Receive files outside the classpath in quarkus-liquibase-mongodb
4 participants
@juanjogv @geoand @gsmet @loicmathieu